Prevent NPE in inheritance assembler - #12632
Conversation
… is root path or artifactId is null
elharo
changed the title
elharo
changed the title
Removed assertNotNull checks for assembler in tests.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The production code fix for the NPE in getChildPathAdjustment() is correct and minimal — the getFileName() != null guard handles root paths where getFileName() returns null, with a sensible fallback to artifactId. However, the test rewrite has a regression issue.
Deleted existing test coverage (high severity)
The PR completely replaces the test file content, deleting testPluginConfiguration() and the entire XML-based test infrastructure (testInheritance helper, getModel, getPom, xmlunit comparison). These tests were added in commit d58c96bc36 by hboutemy specifically to validate reportSet inheritance in the Maven 4 (impl) model builder.
While the compat test at compat/maven-model-builder/.../DefaultInheritanceAssemblerTest.java still uses the XML resources, it exercises the compat (Maven 3) assembler, not the impl (Maven 4) DefaultInheritanceAssembler which uses InheritanceModelMerger — a different code path.
The new NPE regression tests are valuable and should be added alongside the existing tests, not used to replace them.
Minor: trivial assertions (low severity)
The assertNotNull(assembler) calls in each test method only verify that @BeforeEach ran — they don't test the assembler's behavior.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes a NullPointerException in DefaultInheritanceAssembler.getChildPathAdjustment() when the child model’s project directory is the filesystem root (so getFileName() returns null) and/or when artifactId is null, and adds regression tests to cover these scenarios.
Changes:
- Added a null check for
child.getProjectDirectory().getFileName()before calling.toString(). - Replaced prior XML-based inheritance tests with focused regression tests asserting no NPE for the identified edge cases.
- Introduced a test case covering a combined “null artifactId + root project directory” scenario with modules configured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultInheritanceAssembler.java | Adds a guard to prevent NPE when project directory is root and getFileName() is null. |
| impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultInheritanceAssemblerTest.java | Adds regression tests asserting assembling inheritance does not throw for null artifactId and root project directory cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void testAssembleWithNullArtifactIdDoesNotThrowNpe() { | ||
| Model parent = Model.newBuilder() | ||
| .modelVersion("4.0.0") | ||
| .groupId("test") | ||
| .artifactId("parent") | ||
| .version("1.0") | ||
| .build(); | ||
|
|
||
| public void testInheritance(String baseName) throws Exception { | ||
| testInheritance(baseName, false); | ||
| testInheritance(baseName, true); | ||
| } | ||
| Model child = Model.newBuilder() | ||
| .modelVersion("4.0.0") | ||
| .groupId("test") | ||
| .version("1.0") | ||
| .build(); | ||
|
|
||
| public void testInheritance(String baseName, boolean fromRepo) throws Exception { | ||
| Model parent = getModel(baseName + "-parent"); | ||
| Model child = getModel(baseName + "-child"); | ||
| assertDoesNotThrow(() -> assembler.assembleModelInheritance(child, parent, null, null)); | ||
| } |
| .pomFile(Paths.get("/pom.xml")) | ||
| .build(); |
| .modules(List.of("../child/pom.xml")) | ||
| .build(); |
Restore XML-based inheritance tests alongside new NPE tests
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The author has thoroughly addressed the previous review feedback. The deleted testPluginConfiguration test is restored, 7 additional tests are ported from the compat module, and 3 new NPE tests properly cover the fix. The production code change is correct and minimal.
Minor observations (non-blocking):
testAssembleWithNullArtifactIdDoesNotThrowNpeconstructs a parent with no modules, so the for-loop ingetChildPathAdjustmentis never entered and the nullchildNamepath at line 128 is not actually exercised. The third test (testAssembleWithNullArtifactIdAndRootProjectDirectoryDoesNotThrowNpe) is the one that truly exercises the fix with both null conditions.- The compat module's
testFlatTrickyUrls()test is not ported, which is acceptable since it relies on complex assertion logic and the mutable Model API that does not translate directly to the immutable API. Could be considered for a follow-up if desired. - CI is green on all completed checks.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
fixes #12603
Bug
DefaultInheritanceAssembler.getChildPathAdjustment()throws NPE when:child.getProjectDirectory().getFileName()returns null (root path like/)child.getArtifactId()returns nullFix
child.getProjectDirectory().getFileName()before calling.toString()Testing
Added
DefaultInheritanceAssemblerTestwith three test cases:getFileName()returns null)Co-authored-by: elharo